-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Amm liquidity pool #256
Amm liquidity pool #256
Conversation
… amm-liquidity-pool # Conflicts: # contracts/SportMarkets/SportsAMM.sol
… amm-liquidity-pool # Conflicts: # test/contracts/SportMarkets/SportsAMMDiscounts2.js # test/contracts/SportMarkets/SportsAMMDiscounts3.js
… amm-liquidity-pool
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
- Coverage 85.66% 85.02% -0.65%
==========================================
Files 71 74 +3
Lines 2163 2203 +40
Branches 1092 1115 +23
==========================================
+ Hits 1853 1873 +20
- Misses 310 330 +20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
||
/// @notice Deposit funds from user into pool for the next round | ||
/// @param amount Value to be deposited | ||
function deposit(uint amount) external canDeposit(amount) nonReentrant whenNotPaused { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although is cleaner, modifier used only once might produce higher contract size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a reused approach from the vaults code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me.
There is also a room for the borrowing staked amount.
… amm-liquidity-pool # Conflicts: # contracts/SportMarkets/SportsAMM.sol
# Conflicts: # .openzeppelin/unknown-420.json # contracts/EscrowAndStaking/StakingThales.sol
/// @notice Return the end time of the passed round | ||
/// @param round number | ||
/// @return uint the end time of the given round | ||
function getRoundEndTime(uint round) public view returns (uint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to uint _round
since round
is a state variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thx
/// @notice Return the start time of the passed round | ||
/// @param round number | ||
/// @return uint the start time of the given round | ||
function getRoundStartTime(uint round) public view returns (uint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to uint _round
since round
is a state variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
); | ||
} | ||
|
||
uint nextRound = round + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable, delete if not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
uint _roundStartTime, | ||
uint _roundEndTime | ||
) external { | ||
require(!initialized, "Ranged Market already initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thx
/// @param sUSDAmount amount to get for mint | ||
function commitTrade(address market, uint sUSDAmount) external nonReentrant whenNotPaused onlyAMM { | ||
require(started, "Pool has not started"); | ||
require(sUSDAmount > 0, "Can't commit a zero trade"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add require ! zero address for market market
i know it will fail due to ISportPositionalMarket marketContract = ISportPositionalMarket(market);
in getMarketRound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is trust assumed as onlyAMM can call it
/* ========== STATE VARIABLES ========== */ | ||
|
||
ISportsAMM public sportsAMM; | ||
IERC20Upgradeable public sUSD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are only using sUSD? and on ARBI it is USDC right? 1e18 both? if not that case was covered right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there isnt much work to support USDC, I'll try to implement it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added USDC support
refactor obtainer to use normalized odds from storage also resize contract
Release steps: